Skip to content

chore: Add ObjectID to ComputerStatus #251

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Oct 16, 2025
Merged

chore: Add ObjectID to ComputerStatus #251

merged 3 commits into from
Oct 16, 2025

Conversation

@MikeX777
Copy link
Contributor

@MikeX777 MikeX777 commented Oct 2, 2025 •
edited by coderabbitai bot
Loading

Description

Adding ObjectId to ComputerStatus

Motivation and Context

Resolves: BED-6568

How Has This Been Tested?

This has been tested by creating a build of SharpHound with the updated library, and using running a collection.

Screenshots (if appropriate):

Types of changes

  • Chore (a change that does not modify the application functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • Documentation updates are needed, and have been made accordingly.
  • I have added and/or updated tests to cover my changes.
  • All new and existing tests passed.
  • My changes include a database migration.

Summary by CodeRabbit

  • New Features
    • Status outputs now include a computer Object ID, adding an extra column to CSV exports for improved traceability across processors (LDAP, SMB, SPN, Local Groups, Sessions, URA, Cert Abuse).
  • Breaking Changes
    • CSV status rows now contain an additional Object ID column.
    • Some scan operations accept an additional identifier parameter.
  • Tests
    • Unit tests updated to align with new signatures and CSV output.

@MikeX777 MikeX777 self-assigned this Oct 2, 2025
@MikeX777 MikeX777 added the enhancement New feature or request label Oct 2, 2025
@coderabbitai
Copy link

coderabbitai bot commented Oct 2, 2025 •
edited
Loading

Walkthrough

Adds an ObjectId property to CSVComputerStatus, updates its CSV serialization, and propagates that identifier through multiple processors and status reports. Several method signatures were extended to accept object identifiers and corresponding unit tests were adjusted to match the new signatures.

Changes

Cohort / File(s) Summary
Data model: CSVComputerStatus
src/CommonLib/CSVComputerStatus.cs		 Added public string ObjectId { get; set; }; updated ToCsv() to include ObjectId as an additional CSV cell (four cells total).
Processor status payloads
src/CommonLib/Processors/CertAbuseProcessor.cs, src/CommonLib/Processors/ComputerSessionProcessor.cs, src/CommonLib/Processors/DCLdapProcessor.cs, src/CommonLib/Processors/LdapPropertyProcessor.cs, src/CommonLib/Processors/LocalGroupProcessor.cs, src/CommonLib/Processors/SPNProcessors.cs, src/CommonLib/Processors/SmbProcessor.cs, src/CommonLib/Processors/UserRightsAssignmentProcessor.cs		 Populated ObjectId on CSVComputerStatus instances sent from these processors; no other control-flow or error-handling changes.
Public API signature changes
src/CommonLib/Processors/DCLdapProcessor.cs, src/CommonLib/Processors/SmbProcessor.cs, src/CommonLib/Processors/ComputerAvailability.cs		 DCLdapProcessor.Scan(string computerName) → Scan(string computerName, string computerObjectId); SmbProcessor.Scan(string host) → Scan(string host, string securityIdentifier); ComputerAvailability.IsComputerAvailable(..., string objectId = null) added optional parameter.
Unit tests updated
test/unit/DCLdapProcessorTest.cs, test/unit/SmbProcessorTest.cs		 Updated test calls to pass the new additional string parameter (e.g., "") to match changed Scan signatures.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Client
  participant Processor
  participant CSVStatus as CSVComputerStatus
  participant StatusSink

  Client->>Processor: Scan(...) / IsComputerAvailable(..., objectId)
  rect rgba(230,245,255,0.6)
    note right of Processor: Build status payload including ObjectId
    Processor->>CSVStatus: instantiate with { Task, ComputerName, Status, ObjectId }
    CSVStatus-->>Processor: ToCsv() returns CSV with ObjectId
  end
  Processor->>StatusSink: Send(status CSV with ObjectId)
  StatusSink-->>Client: Acknowledgement / Result
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested reviewers

  • definitelynotagoblin

Poem

I thump my paws with merry pride,
A fourth cell joined the CSV ride;
ObjectId hops into each report,
From root to branch, it’s now a part.
Tests and processors dance aligned. 🥕🐇

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly describes the primary change of adding the ObjectId field to ComputerStatus and follows the repository convention of prefixing chores. It is directly related to the main change, clear, and specific enough for a teammate scanning history.
Description Check ✅ Passed The pull request description closely follows the repository’s template by including a detailed Description, linking the related issue in the Motivation and Context section, providing testing details, specifying the change type, and marking the relevant checklist items; optional sections like screenshots are appropriately omitted without impacting completeness.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch mcuomo/BED-6568
📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fccb713 and 4e86fc0.

📒 Files selected for processing (1)
  • src/CommonLib/Processors/LdapPropertyProcessor.cs (2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build
🔇 Additional comments (2)
src/CommonLib/Processors/LdapPropertyProcessor.cs (2)

268-273: LGTM! ObjectId correctly populated from resolved host.

The ObjectId field is appropriately assigned from the validated SecurityIdentifier, which is already confirmed to be successful and contain a valid SID format. This change aligns with the PR objective of adding ObjectId to ComputerStatus.

384-389: LGTM! Consistent implementation of ObjectId assignment.

The ObjectId field is correctly assigned from the validated SecurityIdentifier, following the same pattern as in ReadUserProperties. The implementation is consistent across both methods, ensuring that computer status reports include the object identifier when delegation is configured.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@MikeX777 MikeX777 marked this pull request as ready for review October 4, 2025 01:42
@MikeX777 MikeX777 merged commit 999bf5f into v4 Oct 16, 2025
3 checks passed
@MikeX777 MikeX777 deleted the mcuomo/BED-6568 branch October 16, 2025 18:51
@github-actions github-actions bot locked and limited conversation to collaborators Oct 16, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@definitelynotagoblin definitelynotagoblin definitelynotagoblin approved these changes

Assignees

@MikeX777 MikeX777

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@MikeX777 @definitelynotagoblin